ci: prototype trusted external sweep dispatch / CI：试验可信外部扫描调度 - #2454
Conversation
Route maintainer-approved fork revisions through a main-branch workflow_dispatch so benchmark jobs receive required secrets while pinning the exact approved SHA.\n\n中文：通过 main 分支的 workflow_dispatch 调度维护者批准的 fork 提交，使基准测试任务在固定获批 SHA 的同时获取所需密钥。
Keep the behavioral external-fork gating coverage while dropping string-based checks that could pass despite broken workflow semantics.\n\n中文：保留外部 fork 门控的行为测试，删除即使工作流语义损坏也可能通过的字符串断言。
|
Claude finished @cquil11's task in 7m 43s —— View job Review of PR #2454
Found 1 blocking issue and 1 warning, posted as inline comments. 🔴 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| changelog-base-ref: | ||
| description: "Trusted-dispatch mode: base SHA for process_changelog.py" | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| changelog-head-ref: | ||
| description: "Trusted-dispatch mode: exact approved PR head SHA" | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| trim-conc: | ||
| description: "Trim each configuration to its minimum concurrency" | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| all-evals: | ||
| description: "Expand evaluation selection to every fixed-sequence configuration" | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| evals-only: | ||
| description: "Suppress throughput and run only evaluations" | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| fail-fast: | ||
| description: "Cancel the rest of each matrix after its first failure" | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| pr-labels-json: | ||
| description: "Labels from the source PR for priority scoring" | ||
| required: false | ||
| type: string | ||
| default: "[]" |
There was a problem hiding this comment.
🔴 BLOCKING: The workflow_dispatch trigger now defines 14 inputs, but GitHub enforces a hard maximum of 10 inputs per workflow_dispatch event.
Why it matters: Dispatching this workflow fails with you may only define up to 10 'inputs' for a 'workflow_dispatch' event — which breaks the new trusted-external-sweep dispatcher, the existing claude.yml e2e automation, and manual runs from the Actions UI. Since this PR is already merged, e2e-tests.yml on main is likely un-dispatchable right now; a one-off manual dispatch will confirm.
Fix: The four new booleans are all derivable from pr-labels-json (that's exactly how trusted-external-sweep.yml computes them before dispatching): drop trim-conc, all-evals, evals-only, and fail-fast from the workflow_dispatch inputs, derive them inside get-jobs from the labels JSON (strategy blocks can read needs.get-jobs.outputs.*), and stop passing them from the dispatcher. That brings the count to 10. The workflow_call block has no 10-input limit and can keep them.
| if (!pull.merge_commit_sha) { | ||
| core.setFailed('GitHub has not produced a merge commit; resolve conflicts and re-add the label.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 WARNING: pull.merge_commit_sha is not guaranteed to be the merge of the validated pull.head.sha. GitHub computes the test merge asynchronously, and pulls.get can return a stale merge_commit_sha from a previous head while head.sha already reflects the current one.
Why it matters: An attacker can push a malicious commit (GitHub computes its test merge), then force-push back to the benign head. If the maintainer applies the label during the recompute window, every check here passes — pull.head.sha matches the event SHA — but merge_commit_sha still points at the merge of the malicious head, and that is the exact revision the dispatched workflow checks out and executes with repository secrets. This is the one gap in an otherwise solid SHA-pinning model.
Fix: verify the merge commit's parentage before dispatching:
const {data: mergeCommit} = await github.rest.git.getCommit({
owner: context.repo.owner,
repo: context.repo.repo,
commit_sha: pull.merge_commit_sha,
});
if (!mergeCommit.parents.some((p) => p.sha === pull.head.sha)) {
core.setFailed(
`Merge commit ${pull.merge_commit_sha} does not have approved head ${pull.head.sha} as a parent; ` +
'wait for GitHub to recompute mergeability, then remove and re-add the label.',
);
return;
}
There was a problem hiding this comment.
Beyond the inline findings, I checked whether trusted-external-sweep.yml lacking a concurrency group lets rapid label add/remove race into an unintended dispatch — the script re-fetches the PR and re-verifies the exact head SHA and the single primary label before dispatching, so a stale/duplicate labeling event can't bypass the revision-specific approval; not a real gap.
Extended reasoning...
This run's own finding is significant: the trusted-dispatch changelog path resolves GENERATE_SWEEPS_PY_SCRIPT/MASTER_CONFIGS relative paths against the untrusted fork checkout rather than the pinned .ci-priority root, which undermines the exact isolation this PR is meant to establish for external-fork PRs. That alone warrants human review before merge, so I'm not approving. I additionally looked into the no-concurrency-group question on trusted-external-sweep.yml and ruled it out for the reason above.
| if [ -n "$CHANGELOG_BASE_REF" ] || [ -n "$CHANGELOG_HEAD_REF" ]; then | ||
| if [ -z "$CHANGELOG_BASE_REF" ] || [ -z "$CHANGELOG_HEAD_REF" ]; then | ||
| echo "Both changelog-base-ref and changelog-head-ref are required" >&2 | ||
| exit 1 | ||
| fi | ||
| CMD=( | ||
| uv run --no-project --with pydantic --with pyyaml --python 3.12 | ||
| "${PRIORITY_ROOT}/utils/process_changelog.py" | ||
| --changelog-file "${GITHUB_WORKSPACE}/perf-changelog.yaml" | ||
| --base-ref "$CHANGELOG_BASE_REF" | ||
| --head-ref "$CHANGELOG_HEAD_REF" | ||
| ) | ||
| if [ "$TRIM_CONC" = "true" ]; then | ||
| CMD+=(--trim-conc) | ||
| fi | ||
| if [ "$ALL_EVALS" = "true" ]; then | ||
| CMD+=(--all-evals) | ||
| fi | ||
| if [ "$EVALS_ONLY" = "true" ]; then | ||
| CMD+=(--evals-only) | ||
| fi | ||
| RAW_CONFIG_JSON=$("${CMD[@]}") | ||
| CONFIG_JSON=$(python3 -c 'import json,sys; data=json.load(sys.stdin); rows=[row for family in ("single_node","multi_node") for group in data.get(family,{}).values() for row in group]; rows.extend(row for family in ("evals","agentic_evals","multinode_evals") for row in data.get(family,[])); print(json.dumps(rows))' <<<"$RAW_CONFIG_JSON") | ||
| else | ||
| GENERATE_COMMAND="${{ inputs.generate-cli-command || github.event.inputs.generate-cli-command }}" | ||
| if [ -z "$GENERATE_COMMAND" ]; then | ||
| echo "generate-cli-command is required outside trusted changelog dispatch mode" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🔴 The trusted-dispatch get-jobs step invokes the pinned .ci-priority/utils/process_changelog.py, but that script's internal subprocess.run/config-load calls use the repo-relative paths utils/matrix_logic/generate_sweep_configs.py and configs/{amd,nvidia}-master.yaml (from utils/constants.py) with no cwd= override, so they resolve against the step's actual working directory — the untrusted external-fork checkout at GITHUB_WORKSPACE — rather than the trusted .ci-priority copy. This lets an external contributor supply their own generate_sweep_configs.py and/or a doctored configs/*-master.yaml in their fork, defeating the config validation and matrix generation that the .ci-priority isolation was built to protect, with the resulting matrix flowing into downstream secrets: inherit benchmark jobs. Fix by passing cwd=PRIORITY_ROOT (or absolutizing GENERATE_SWEEPS_PY_SCRIPT/MASTER_CONFIGS) in utils/process_changelog.py, or by exporting PRIORITY_ROOT for the subprocess to use.
Extended reasoning...
The mechanism, confirmed against the actual source. utils/process_changelog.py (unmodified by this PR) imports GENERATE_SWEEPS_PY_SCRIPT = "utils/matrix_logic/generate_sweep_configs.py" and MASTER_CONFIGS = ["configs/amd-master.yaml", "configs/nvidia-master.yaml"] from utils/constants.py" — both repo-relative. Line 173 calls master_config = load_config_files(MASTER_CONFIGS)to validate that every changelog entry'sconfig-keysexist in the master config (raisingValueErrorotherwise, lines 104-119). Lines 211-233 and 248-274 then callsubprocess.run(["python3", GENERATE_SWEEPS_PY_SCRIPT, "test-config", ..., "--config-files", MASTER_CONFIGS, ...])with nocwd=argument, so both the script path and the config-file arguments resolve against whatever directory the parent process is running in.\n\nIne2e-tests.yml's get-jobsstep, that directory isGITHUB_WORKSPACE. In trusted-dispatch mode the 'Checkout code (ref)' step checks out inputs.ref (pull.merge_commit_sha, the *untrusted* external-fork merge revision) directly into GITHUB_WORKSPACEwith nopath:override, while the trusted tooling is separately checked out under.ci-priority(pinned togithub.workflow_sha). PRIORITY_ROOTis correctly set to.ci-priority, so process_changelog.pyitself — and its own top-level imports, which resolve viasys.path[0] = the script's own directory — do run the trusted copy. But the *relative* paths inside it (GENERATE_SWEEPS_PY_SCRIPT, MASTER_CONFIGS) are resolved by Python/the OS against the process cwd, which is GITHUB_WORKSPACE— the untrusted checkout — notPRIORITY_ROOT.\n\n**Why this defeats the isolation, not just 'runs PR code.'** trusted-external-sweep.yml always dispatches e2e-tests.ymlwithchangelog-base-ref/changelog-head-refset, so every approved external-fork run takes this branch unconditionally. Two independent attacks follow from the same root cause: (1) the external contributor can put arbitrary Python in their ownutils/matrix_logic/generate_sweep_configs.py, which then executes inside the trusted, secrets-context e2e-tests.ymlrun; and (2), even without any code execution, the contributor can simply edit their ownconfigs/amd-master.yaml/configs/nvidia-master.yamlto add a fabricated config entry (e.g. with an attacker-controlled containerimage) and reference that key from perf-changelog.yaml. Because load_config_files(MASTER_CONFIGS)reads the *fork's* master-config files, the fabricated key passes the 'must exist in master configs' check that.ci-priority's trusted process_changelog.pyis supposed to enforce, andgenerate_sweep_configs.py(also resolved from the fork) will happily emit a matrix row carrying that entry. That row is exactly the JSON thatget-jobsoutputs and that thetest-sweep-jobs consume withsecrets: inherit.\n\n**Why nothing else in the workflow catches this.** The maintainer's trust decision, per the PR description, is scoped to 'the approved merge revision' being executed by the benchmark jobs — but the entire reason process_changelog.pyandci_priority.pyare routed through.ci-priorityin the first place is to keep the *matrix-generation and priority-scoring control plane* on trusted code so an external label-approval doesn't also hand the contributor free rein over what configuration/image ends up in the secrets-bearing matrix. That is precisely the protection this bug removes for two of.ci-priority's three components (generate_sweep_configs.pyand the master configs;ci_priority.pyalone remains safely pinned since it's invoked with an explicit"/utils/ci_priority.py"path and--policy "/configs/ci-priority.yaml"). It is true that the get-jobs step itself has no secrets in its own env (persist-credentials: false, workflow-level permissions: contents: read), so this is not a direct secrets-exfiltration primitive from inside get-jobs— but that's not the threat this isolation exists to prevent. The threat is exactly what this bug enables: attacker-influenced data/code reaching the config-generation step that feedssecrets: inheritGPU jobs downstream.\n\n**Step-by-step PoC.** (1) An external contributor forks the repo and, in their PR branch, adds a new keyevil-keytoconfigs/nvidia-master.yamlwhoseimagefield points at a Docker image they control (crafted to exfiltrate the runner's inherited secrets or otherwise misbehave). (2) They add a corresponding entry inperf-changelog.yamlreferencingconfig-keys: [evil-key]. (3) A maintainer, believing they are approving only the exact reviewed source diff, applies full-sweep-fail-fast(one of the primary sweep labels) to the PR. (4)trusted-external-sweep.ymlvalidates the maintainer's permission and the head SHA, then dispatchese2e-tests.ymlonmainwithref=pull.merge_commit_sha, changelog-base-ref, changelog-head-ref. (5) In get-jobs, GITHUB_WORKSPACEis checked out to the merge commit (containing the attacker'sevil-keyentry and doctored master config);.ci-priorityholds the trustedprocess_changelog.py. (6) process_changelog.pyruns from.ci-prioritybut callsload_config_files(MASTER_CONFIGS)andsubprocess.run(["python3", GENERATE_SWEEPS_PY_SCRIPT, ...])with cwd defaulting toGITHUB_WORKSPACE— the attacker's checkout — soevil-keyis found 'valid' and a matrix row containing the attacker's image is generated. (7) That row is emitted viaGITHUB_OUTPUT, consumed by e.g. test-sweep-single-node, which runs benchmark-tmpl.ymlwithsecrets: inheritand the attacker-chosenimage. The maintainer never intended to approve a new config entry or image — only the reviewed diff — yet it reaches a secrets-bearing job.\n\n**Fix.** Pass cwd=os.environ["PRIORITY_ROOT"](or an equivalent absolute base) to bothsubprocess.runcalls inutils/process_changelog.py, and resolve MASTER_CONFIGS/GENERATE_SWEEPS_PY_SCRIPTrelative to that same trusted root before callingload_config_files/subprocess.run`, rather than relying on the ambient process cwd.
| labels = set(sc.get("labels", [])) | ||
| draft = sc.get("draft", False) | ||
| is_pr = sc["event"] == "pull_request" | ||
| is_internal_pr = sc.get("head_repo", "SemiAnalysisAI/InferenceX") == ( | ||
| "SemiAnalysisAI/InferenceX" | ||
| ) | ||
| action = sc.get("action") | ||
|
|
||
| check_runs = ( |
There was a problem hiding this comment.
🟡 The exhaustive cross-product test in test_run_sweep_gating.py never varies the new head_repo/fork axis, so all 17666 generated scenarios stay on the internal repo and the newly-added is_internal_pr check in reference_gate() is only ever exercised as trivially-true. Only the single hand-written PR-sync-external-fork-defers-to-trusted-dispatch case covers the fork path, so an interaction bug between is_internal_pr and other action/label/draft axes wouldn't be caught by the exhaustive sweep. Consider adding head_repo as an 8th axis to pr_axes and bumping the asserted scenario count.
Extended reasoning...
What the bug is. This PR adds an is_internal_pr clause to reference_gate() in utils/changelog_gate_tests/test_run_sweep_gating.py, mirroring the new github.event.pull_request.head.repo.full_name == github.repository check the PR adds to run-sweep.yml's setup job. The module's docstring claims this harness "covers every distinct skip/run decision" and "cannot drift from production" because it exhaustively cross-checks reference_gate() against the real parsed if conditions over every combination of input axes. But _all_scenarios() builds pr_axes from only 7 axes — action, draft, labels, label.name, reuse_auth, check, msg — and never includes head_repo. Every constructed scenario dict omits the head_repo key entirely.
Code path. Both _ctx() (used by run_dag(), which evaluates the real production if strings) and reference_gate() fall back to sc.get('head_repo', 'SemiAnalysisAI/InferenceX') when the key is absent. Since _all_scenarios() never sets head_repo, every one of the 17666 generated scenarios defaults to the internal repo on both sides of the comparison, making is_internal_pr (and the mirrored production clause) vacuously True in every generated case. test_exhaustive_cross_product() even hard-asserts len(scenarios) == 17666, the same count as before the fork dimension existed, confirming no doubling from a fork axis.
Why existing coverage doesn't catch this. The PR does add one hand-written named case, PR-sync-external-fork-defers-to-trusted-dispatch, which does set head_repo: 'external/InferenceX' and correctly asserts ('success', 'success', 'SKIP'). That case is checked by test_gating_decision and test_named_cases_match_reference_spec, so the primary intended behavior (external fork PRs defer to the trusted dispatcher) is verified. However, the exhaustive cross-product sweep — the part of the suite explicitly designed to catch any divergence between reference_gate()'s encoding of intent and the real parsed YAML if string, across the full combinatorial space — never varies this dimension. If a future edit introduced a subtle interaction bug, e.g. is_internal_pr accidentally short-circuiting the [skip-sweep] check, or interacting incorrectly with unlabeled/labeled actions or draft state when the PR is external, the exhaustive test would not detect it, because it never generates a scenario where is_internal_pr is False.
Impact. This is a test-only coverage gap, not a runtime bug — is_internal_pr is a single straightforward AND clause and the production behavior for the fork case is separately verified by the named scenario and indirectly by the dedicated trusted-external-sweep.yml workflow. But it does mean the module's own stated guarantee ("covers every distinct skip/run decision... cannot drift from production") is no longer accurate for this dimension, and the gap is easy to overlook precisely because everything still passes.
Fix. Add head_repo as an 8th axis to pr_axes in _all_scenarios(), e.g. [SemiAnalysisAI/InferenceX, external/InferenceX], thread it into each generated scenario dict, and update the hard-coded assert len(scenarios) == 17666 to reflect the doubled PR-scenario count (17664 × 2 + 2 push cases = 35330).
Step-by-step proof.
_all_scenarios()constructspr_axes = itertools.product(actions, draft, label_cfgs, label_names, reuse_auth, check, msg)— 7 axes, nohead_repo.- Each scenario dict is built as
{"event": "pull_request", "action": a, "draft": d, "labels": labs, "label_name": ln, "reuse_auth": r, "check": chk, "msg": msg}— again, nohead_repokey. _ctx(sc)computesgithub.event.pull_request.head.repo.full_nameviasc.get("head_repo", "SemiAnalysisAI/InferenceX")→ always resolves to the internal repo for every generated scenario.reference_gate(sc)computesis_internal_prthe same way, via the same default → alwaysTruefor every generated scenario.- Therefore, for all 17664 generated PR scenarios,
is_internal_prisTrueon both the production-condition side (run_dag) and the reference side (reference_gate), so the new clause never actually gets exercised asFalseby the sweep — a regression whereis_internal_prbreaks some other combination (e.g. is inverted, or drops theand) would still passtest_exhaustive_cross_product()today. - Confirmed by the count assertion:
assert len(scenarios) == 17666is unchanged from its pre-PR value, proving no new dimension was folded into the product.
* ci: prototype trusted external sweep dispatch Route maintainer-approved fork revisions through a main-branch workflow_dispatch so benchmark jobs receive required secrets while pinning the exact approved SHA.\n\n中文：通过 main 分支的 workflow_dispatch 调度维护者批准的 fork 提交，使基准测试任务在固定获批 SHA 的同时获取所需密钥。 * test: remove shallow workflow assertions Keep the behavioral external-fork gating coverage while dropping string-based checks that could pass despite broken workflow semantics.\n\n中文：保留外部 fork 门控的行为测试，删除即使工作流语义损坏也可能通过的字符串断言。
1 participant
Summary
pull_request_targetworkflow for external fork PRse2e-tests.ymlfrommainso benchmark jobs can receive repository secretspull_requestruns limited to changelog/reuse validation instead of launching GPU jobs with empty secretsSecurity model
The
pull_request_targetjob never checks out or executes PR code. It verifies the labeling actor's repository permission, refetches the PR, confirms the exact event head SHA is still current, and only then dispatches the trusted workflow. The benchmark workflow intentionally executes the approved merge revision with secrets; applying the primary label is therefore an explicit trust decision for that exact SHA. Later pushes require removing and re-adding the label.PoC limitations
/reuse-sweep-runrun-sweep.yml's canary-first sequencingValidation
actionlintpasses for all changed workflows740e35381..c6e4c333changelog path generates 9dsv4/gb200rows摘要
pull_request_target工作流main调度e2e-tests.yml，使基准测试任务能够获取仓库密钥pull_request运行只执行 changelog/复用校验，不再使用空密钥启动 GPU 任务安全模型
pull_request_target任务不会检出或执行 PR 代码。它会验证添加标签者的仓库权限、重新获取 PR、确认事件中的 head SHA 仍是当前版本，然后才调度可信工作流。基准测试工作流会有意使用密钥执行获批的合并版本，因此应用主标签就是对该精确 SHA 的明确授权。后续推送必须移除并重新添加该标签。PoC 限制
/reuse-sweep-runrun-sweep.yml的 canary-first 顺序验证
actionlint740e35381..c6e4c333changelog 路径生成 9 个dsv4/gb200条目